Improve names of .node files by Bashamega · Pull Request #768 · microsoft/node-pty

Bashamega · 2025-03-14T12:33:17Z

Tyriar · 2025-03-14T12:41:39Z

binding.gyp

          'target_name': 'conpty',
          'sources' : [
-            'src/win/conpty.cc',
+            'src/win/conpty_backend.cc',


Is it target_name we need to change as well to change the .node file, then an update here is needed too:

node-pty/src/windowsPtyAgent.ts

Lines 64 to 93 in 467cea0

if (!conptyNative) {

try {

conptyNative = require('../build/Release/conpty.node');

} catch (outerError) {

try {

conptyNative = require('../build/Debug/conpty.node');

} catch (innerError) {

console.error('innerError', innerError);

// Re-throw the exception from the Release require if the Debug require fails as well

throw outerError;

}

}

}

} else {

if (!winptyNative) {

try {

winptyNative = require('../build/Release/pty.node');

} catch (outerError) {

try {

winptyNative = require('../build/Debug/pty.node');

} catch (innerError) {

console.error('innerError', innerError);

// Re-throw the exception from the Release require if the Debug require fails as well

throw outerError;

}

}

}

}

this._ptyNative = this._useConpty ? conptyNative : winptyNative;

Thank you. I will update it

We just need to figure out why CI is failing now. See the error at https://dev.azure.com/vscode/node-pty/_build/results?buildId=147363&view=logs&j=0bc77094-9fcd-5c38-f6e4-27d2ae131589&t=2917e7a9-bb9d-585b-890e-1ad69b6f3f65&l=976

Name should be updated in

node-pty/scripts/post-install.js

Lines 10 to 11 in 090384f

path.join(RELEASE_DIR, 'conpty.node'),

path.join(RELEASE_DIR, 'conpty.pdb'),

to fix the build error.

Hello @deepak1556
I have updated the post-install script, but it is still not working.

Please check #768 (comment)

Bashamega · 2025-03-14T14:43:04Z

Hello @Tyriar
I don't know why it is not working, it is working on Linux and Mac.
I don't know c++ or how this project is built.
Can you help me?
Thanks.

Tyriar · 2025-03-14T15:41:58Z

That error means that the expected compile .node file isn't there, so the change in binding.gyp didn't seem to do the right file or something. I don't have time to look into this further atm unfortunately.

Bashamega · 2025-03-14T15:47:11Z

That error means that the expected compile .node file isn't there, so the change in binding.gyp didn't seem to do the right file or something. I don't have time to look into this further atm unfortunately.

I will try to look at it more

deepak1556 · 2025-03-15T04:10:30Z

binding.gyp

+          'target_name': 'conpty_backend',
          'sources' : [
-            'src/win/conpty.cc',
+            'src/win/conpty_backend.cc',


changing the target name is sufficient to achieve the required result, lets keep the source file names unchanged.

deepak1556 · 2025-03-15T04:10:50Z

binding.gyp

          ],
          'sources' : [
-            'src/win/winpty.cc',
+            'src/win/winpty_backend.cc',


Not required for the scope of this PR, lets revert this.

deepak1556 · 2025-03-17T02:41:19Z

scripts/post-install.js

-    const destFolder = path.join(RELEASE_DIR, 'conpty');
+    const destFolder = path.join(RELEASE_DIR, 'conpty_backend');
    fs.mkdirSync(destFolder, { recursive: true });
-    for (const file of ['conpty.dll', 'OpenConsole.exe']) {


These should not be renamed, they are prebuilt binaries from conpty library.

deepak1556 · 2025-03-17T11:12:33Z

There is one another place to update

node-pty/src/win/conpty.cc

Line 171 in bfbd943

HMODULE hModule = GetModuleHandleA("conpty.node");

and that should fix the failing tests.

Bashamega added 4 commits March 14, 2025 14:26

Rename conpty.cc to conpty_backend.cc

c9191b8

Rename winpty.cc to winpty_backend.cc

14219fd

patch:file name

6a5f6cd

Update binding.gyp

0b03874

Tyriar requested changes Mar 14, 2025

View reviewed changes

Bashamega added 2 commits March 14, 2025 15:20

Update binding.gyp

8e7be77

Update windowsPtyAgent.ts

92e4bb1

deepak1556 reviewed Mar 15, 2025

View reviewed changes

Bashamega added 4 commits March 15, 2025 06:34

chore: revert file name change

07e161b

chore: fix post install

d252340

-

c42caca

-

5402d97

Tyriar assigned deepak1556 Mar 15, 2025

deepak1556 reviewed Mar 17, 2025

View reviewed changes

Bashamega added 2 commits March 17, 2025 06:17

Update post-install.js

503b113

Merge branch 'main' into Bashamega-patch-1

407dc48

Bashamega added 2 commits March 18, 2025 07:23

Update conpty.cc

519a8ae

Merge branch 'main' into Bashamega-patch-1

41b5881

	if (!conptyNative) {
	try {
	conptyNative = require('../build/Release/conpty.node');
	} catch (outerError) {
	try {
	conptyNative = require('../build/Debug/conpty.node');
	} catch (innerError) {
	console.error('innerError', innerError);
	// Re-throw the exception from the Release require if the Debug require fails as well
	throw outerError;
	}
	}
	}
	} else {
	if (!winptyNative) {
	try {
	winptyNative = require('../build/Release/pty.node');
	} catch (outerError) {
	try {
	winptyNative = require('../build/Debug/pty.node');
	} catch (innerError) {
	console.error('innerError', innerError);
	// Re-throw the exception from the Release require if the Debug require fails as well
	throw outerError;
	}
	}
	}
	}
	this._ptyNative = this._useConpty ? conptyNative : winptyNative;

	path.join(RELEASE_DIR, 'conpty.node'),
	path.join(RELEASE_DIR, 'conpty.pdb'),

Conversation

Bashamega commented Mar 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Mar 14, 2025

Uh oh!

Tyriar commented Mar 14, 2025

Uh oh!

Bashamega commented Mar 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepak1556 commented Mar 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants